Skip to content

refactor: extract duplicated alert command execution paths#281

Merged
TheWitness merged 1 commit intoCacti:developfrom
somethingwithproof:refactor/extract-alert-command-execution
Mar 16, 2026
Merged

refactor: extract duplicated alert command execution paths#281
TheWitness merged 1 commit intoCacti:developfrom
somethingwithproof:refactor/extract-alert-command-execution

Conversation

@somethingwithproof
Copy link
Contributor

@somethingwithproof somethingwithproof commented Mar 7, 2026

Summary

  • extract ticket command execution into a shared helper in functions.php
  • extract alert command execution and logging into a shared helper in functions.php
  • replace duplicated command blocks in syslog_process_alerts() method branches with helper calls
  • keep existing command behavior and log formats intact
  • add regression test to enforce helper presence and call-site usage
  • update changelog with issue refactor: extract duplicated alert command execution paths in syslog_process_alerts #278

Fixes #278

Validation

  • php -l functions.php
  • php -l tests/regression/issue278_command_execution_refactor_test.php
  • php tests/regression/issue278_command_execution_refactor_test.php

Copilot AI review requested due to automatic review settings March 7, 2026 03:17
Copilot AI reviewed Mar 7, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses issue #278 by extracting two duplicated command execution code blocks within syslog_process_alert() into shared helper functions in functions.php, reducing maintenance risk and preventing future divergence between the two branches. A regression test validates the refactoring structure.

Changes:

  • Introduces syslog_execute_ticket_command() and syslog_execute_alert_command() as shared helpers in functions.php, replacing two near-identical inline blocks in each branch of syslog_process_alert().
  • Adds a regression test (issue278_command_execution_refactor_test.php) that uses string-matching on functions.php to verify helper presence and call-site count.
  • Updates CHANGELOG.md with issue #278 entry.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
functions.php Adds two new helper functions above syslog_process_alerts() and replaces the two duplicated inline command blocks at the call sites
tests/regression/issue278_command_execution_refactor_test.php New regression test verifying helpers exist and are called from both branches via text-based string matching
CHANGELOG.md Adds changelog entry for issue #278

@somethingwithproof
Copy link
Contributor Author

somethingwithproof commented Mar 7, 2026

Fixed in 70b2147 -- added PHPDoc blocks to both execute helpers matching the existing convention; narrowed the test assertion from the broad prefix to the full call-site signature (including trailing ;) so the count excludes the function definition line.

@somethingwithproof somethingwithproof force-pushed the refactor/extract-alert-command-execution branch 2 times, most recently from f7966e5 to a14e876 Compare March 8, 2026 01:35
@TheWitness
Copy link
Member

TheWitness commented Mar 10, 2026

@somethingwithproof more conflicts with the ChangeLog. I like this change.

TheWitness
TheWitness requested changes Mar 10, 2026
View reviewed changes
Copy link
Member

@TheWitness TheWitness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change. Fix the changelog merge conflicts and this one is okay.

@somethingwithproof somethingwithproof force-pushed the refactor/extract-alert-command-execution branch from 9ed829f to 31d898e Compare March 10, 2026 20:30
@somethingwithproof
Copy link
Contributor Author

somethingwithproof commented Mar 10, 2026

Rebased and CHANGELOG conflicts resolved.

somethingwithproof added a commit to somethingwithproof/plugin_syslog that referenced this pull request Mar 10, 2026
@somethingwithproof
refactor: extract duplicated alert command execution paths
c8fd4cc 
Refs Cacti#281

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the refactor/extract-alert-command-execution branch from 31d898e to c8fd4cc Compare March 10, 2026 20:55
@somethingwithproof somethingwithproof mentioned this pull request Mar 11, 2026
Closed
@somethingwithproof somethingwithproof force-pushed the refactor/extract-alert-command-execution branch from 5945930 to c8fd4cc Compare March 12, 2026 00:21
somethingwithproof added a commit to somethingwithproof/plugin_syslog that referenced this pull request Mar 15, 2026
@somethingwithproof
refactor: extract duplicated alert command execution paths
03eaf85 
Refs Cacti#281

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the refactor/extract-alert-command-execution branch from c8fd4cc to 03eaf85 Compare March 15, 2026 05:17
@somethingwithproof
refactor: extract duplicated alert command execution paths
f371223 
Refs Cacti#281

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the refactor/extract-alert-command-execution branch from 03eaf85 to f371223 Compare March 16, 2026 17:10
@TheWitness TheWitness merged commit 9f6715d into Cacti:develop Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@TheWitness TheWitness TheWitness approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

refactor: extract duplicated alert command execution paths in syslog_process_alerts

3 participants

@somethingwithproof @TheWitness